Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

web-wallet: Overhaul landing page #1536

Closed
wants to merge 1 commit into from
Closed

Conversation

nortonandreev
Copy link
Contributor

@nortonandreev nortonandreev commented Mar 12, 2024

Resolves #1534

Screenshot 2024-03-14 at 15 55 58

@nortonandreev nortonandreev self-assigned this Mar 12, 2024
@nortonandreev nortonandreev force-pushed the feature-1534 branch 6 times, most recently from de40a2c to a840ebb Compare March 14, 2024 13:22
@nortonandreev nortonandreev marked this pull request as ready for review March 14, 2024 13:23
@nortonandreev nortonandreev changed the title web-wallet: Overhaul landing page [WIP] web-wallet: Overhaul landing page Mar 14, 2024
@nortonandreev nortonandreev force-pushed the feature-1534 branch 2 times, most recently from a5523ee to 6c9e00f Compare March 14, 2024 14:11
@nortonandreev
Copy link
Contributor Author

Wondering if it won't be better to just have the landing page under the / route now?

Resolves #1533, Resolves #1534, Resolves #1535
Copy link
Contributor

@ascartabelli ascartabelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some notes as there are some problems.

  • these changes should have been made in three separate PRs

  • pressing enter in the unlock textbox doesn't always unlock the wallet:
    in some cases it seems that it just fires a blur event on the textbox

  • if I set up a password, enter my wallet, go to settings and reset the wallet

    • login info is not deleted: new bug? regression we had from before?
    • the system sees existing login info, unlock screen asks for password,
      but by entering the password I'm redirected to the terms & privacy policy in the restore flow.
      By accepting the terms I see my password in clear as the first word of the mnemonic (see screenshot)
      with no possibility to advance further.
Screenshot 2024-03-15 alle 08 12 59
  • if I enter a new mnemonic and with an existing wallet, and I accept to proceed by restoring another wallet,
    I'm brought to the "enter mnemonic phrase" step of the restore flow, but the mnemonic is now in clear
    while just seconds ago it was hidden.
    - should we skip this step?
    - should we show it, but obfuscated by default with an option to show it? Should we consider always obfuscating it by default (or maybe only after a paste) as others are doing? (this is for a new issue anyway)

  • didn't do a full review of the code yet, as there are some problems and we're waiting for @ZER0's input,
    but I think FieldButtonGroup is quite a confusing name for that component.
    Given that for now the component is specific to the application, I would go for something along the lines of
    "UnlockTexbox", or anyway something mentioning that is the control for unlocking.

  • at this point it's probably better to move the TermsOfService in the app components?

  • it's fine having a redirect(301, "/") in the setup route right now, but maybe we should consider
    trapping all 404s in client routing instead and make the same redirect to the home? (in a future issue)
    What do you think?

@nortonandreev
Copy link
Contributor Author

Leaving some notes as there are some problems.

  • these changes should have been made in three separate PRs

We could have done, but I also see the work around the login page as a whole. For the rest of the allowing a sync from a custom block height epic, I will work with Matteo to organize the features in a way that makes sense to deliver them.

From yesterday's discussion, seems like we could do the individual parts separately – I would have personally used an feature branch to capture the whole initiative (with smaller branches containing the individual tasks), so to have the whole thing delivered at once.

  • pressing enter in the unlock textbox doesn't always unlock the wallet:
    in some cases it seems that it just fires a blur event on the textbox

Will investigate. I think I saw some issues with Chrome, which were not present in Safari.

  • if I set up a password, enter my wallet, go to settings and reset the wallet

    • login info is not deleted: new bug? regression we had from before?
    • the system sees existing login info, unlock screen asks for password,
      but by entering the password I'm redirected to the terms & privacy policy in the restore flow.
      By accepting the terms I see my password in clear as the first word of the mnemonic (see screenshot)
      with no possibility to advance further.

I haven't changed anything in the settings page, but I also did not manually test this flow. This is a good catch, I will check what is going on. Also, seeing your password as the first word of the mnemonic, obviously, shouldn't happen, will fix this.

  • if I enter a new mnemonic and with an existing wallet, and I accept to proceed by restoring another wallet,
    I'm brought to the "enter mnemonic phrase" step of the restore flow, but the mnemonic is now in clear
    while just seconds ago it was hidden.
    • should we skip this step?
    • should we show it, but obfuscated by default with an option to show it? Should we consider always obfuscating it by default (or maybe only after a paste) as others are doing? (this is for a new issue anyway)

As you said, let's open a new issue and involve the others in the discussion.

  • didn't do a full review of the code yet, as there are some problems and we're waiting for @ZER0's input

I have pinged Matteo on Discord to have a look. I assume it will be after the consensus update has been rolled out.

but I think FieldButtonGroup is quite a confusing name for that component.
Given that for now the component is specific to the application, I would go for something along the lines of
"UnlockTexbox", or anyway something mentioning that is the control for unlocking.

It is not specific to the application, the same component will be used in the Explorer too, thus the generic component name. Another option could be TextButtonCombo?

  • at this point it's probably better to move the TermsOfService in the app components?

I'm fine with that.

  • it's fine having a redirect(301, "/") in the setup route right now, but maybe we should consider
    trapping all 404s in client routing instead and make the same redirect to the home? (in a future issue)
    What do you think?

I was just thinking from SEO perspective – if a search engine has indexed a route that we have gotten rid of, I think we should respond with the appropriate code – at least initially when we release the change.

@ascartabelli
Copy link
Contributor

Wondering if it won't be better to just have the landing page under the / route now?

I think so, yes.

(cc @kieranhall @deuch13 )

@deuch13
Copy link
Contributor

deuch13 commented Mar 15, 2024

Wondering if it won't be better to just have the landing page under the / route now?

I think so, yes.

(cc @kieranhall @deuch13 )

I agree with this, as I said in my earlier comment I would go even further and have both login and restore flow under the same route.

@ascartabelli
Copy link
Contributor

but I think FieldButtonGroup is quite a confusing name for that component.
Given that for now the component is specific to the application, I would go for something along the lines of
"UnlockTexbox", or anyway something mentioning that is the control for unlocking.

It is not specific to the application, the same component will be used in the Explorer too, thus the generic component name. Another option could be TextButtonCombo?

I thought it was under $lib/components but, anyway, it deserves a better name.
Not sure about your suggestion, but right now nothing good comes to mind: maybe @kieranhall and @deuch13 can barge in and help.

  • it's fine having a redirect(301, "/") in the setup route right now, but maybe we should consider
    trapping all 404s in client routing instead and make the same redirect to the home? (in a future issue)
    What do you think?

I was just thinking from SEO perspective – if a search engine has indexed a route that we have gotten rid of, I think we should respond with the appropriate code – at least initially when we release the change.

Yes, let's leave it like you did as right now we don't have the option to modify things server side, and this way the route exists even for the server.

@nortonandreev nortonandreev marked this pull request as draft March 15, 2024 15:35
@herr-seppia herr-seppia added the module:web-wallet Issues related to web-wallet module label Jun 20, 2024
@nortonandreev
Copy link
Contributor Author

Closing this for the time being, as this has been put on hold without clear idea on when the change will be needed.

@nortonandreev nortonandreev deleted the feature-1534 branch October 7, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:web-wallet Issues related to web-wallet module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

web-wallet: Redesign the landing page with quick access to creating, resetting, and unlocking a Wallet
4 participants